Use subtest_streamed for the sequential executor #107
Conversation
|
This seems fine to me. This doesn't seem like it would break anyone's use of this package. I just pushed a CI fix to master, so you'll need to rebase off master to get the tests to run. As far as dumping the events, I think I used https://metacpan.org/pod/Test2::Tools::EventDumper when I converted TCM to Test2. The author has marked it as deprecated but I think it'll still work for your purposes. I also wrote a From a quick look, I don't think the changes will be too terrible. It looks like this just adds an additional Note event at the beginning of every subtest. |
oschwald
force-pushed
the
greg/streamed-subtests
branch
from
289fc62 to
ae5bc9c
Compare
Codecov Report
@@ Coverage Diff @@
## master #107 +/- ##
==========================================
+ Coverage 92.89% 93.91% +1.02%
==========================================
Files 92 92
Lines 2856 3336 +480
Branches 142 142
==========================================
+ Hits 2653 3133 +480
Misses 148 148
Partials 55 55
Continue to review full report at Codecov.
|
|
I am not sure how to read the two failures, but I think they are unrelated. |
|
If you're not rebased off the latest master that might explain the test failure. Otherwise it means you need to run |
|
It might be nice to get rid of the |
|
There were two |
oschwald
force-pushed
the
greg/streamed-subtests
branch
from
0150a12 to
011bd7e
Compare
|
That |
|
Yeah, the prefix is coming from |
oschwald
changed the title
|
I realized that I had failed to push my last commit. I pushed it now. I think this is ready for review. |
There was a problem hiding this comment.
Instead of splitting all the test classes' expected_test_events into two methods, I think it'd be better to just pass in an argument indicating that the tests are being run in parallel.
AFAICT, all you'd need to do for this flag is something like this:
event Note => sub {
call message => 'TestsFor::Basic';
} unless $is_parallel;This will make it much simpler to change these tests in the future if that's needed.
|
My feeling was that the tests are already extremely hard to follow and update, and introducing arguments like that would make the situation even worse. Right now it is somewhat doable to update the tests by dumping the huge set of events, but once you introduce arguments and the logic for the number of cases that the two event trees differ, then that will be impossible. That said, I think this whole method of testing even outputs is misguided. It makes the tests brittle and hard to update. |
Do you have any ideas on how to make the tests easier to work with? One possibility that comes to mind is to only test the full set of events for a few things, and then just look for a smaller subset in most tests, since we're often just checking a specific portion of the test output in each test. |
2 participants
We have been suffering with #100 since upgrading to a modern version of
Test::Class::Moose. We currently use a hacked up formatter to avoid the issue, but that is not ideal.This change makes the sequential executor use
subtest_streamedinstead ofasync_subtest. In my testing, this works as expected.The tests have not been updated yet as it will be fairly extensive to change all of the
test_events_ischecks. I wanted to make sure the general approach was acceptable first. Also, if there is an easy way to dump the events for those checks, it would likely save quite a bit of manual labor.